Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update ToSnakeCase so it wouldn't return "image_u_r_l" for "imageURL" #4608

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hvuhsg
Copy link

@hvuhsg hvuhsg commented May 6, 2024

fixes #4531

Currently any filename and variable name that is converted to snake case get a separator (default "_") before every uppercase letter excluding some edge cases.

So having the filename "NotrixSDK" will become "notrix_s_d_k" and not the desired "notrix_sdk".

This PR is fixing it by checking if the previous letter is upper case and in this case it does not add the separator.

@hvuhsg hvuhsg requested a review from a team as a code owner May 6, 2024 16:12
@hvuhsg
Copy link
Author

hvuhsg commented May 6, 2024

@microsoft-github-policy-service agree

@hvuhsg hvuhsg changed the title Update ToLowerCase so it wouldn't return "image_u_r_l" for "imageURL" Update ToSnakeCase so it wouldn't return "image_u_r_l" for "imageURL" May 6, 2024
Copy link
Member

@baywet baywet left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the contribution! Can you please also add an entry in the changelog (unreleased, changed)

@hvuhsg
Copy link
Author

hvuhsg commented May 6, 2024

Updated changelog

auto-merge was automatically disabled May 7, 2024 07:09

Pull Request is not mergeable

Copy link
Member

@andrueastman andrueastman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hvuhsg Thanks for the contribution.

Any chance you can double check the failing tests as well?

@hvuhsg
Copy link
Author

hvuhsg commented May 7, 2024

I checked the tests, It seems to have many dependencies for this format "4XX" -> "4_x_x" and I do not have the expertise or the time to update them.

I will not be continuing this PR I hop you could take it from here.

@baywet
Copy link
Member

baywet commented May 7, 2024

@andrueastman it seems the current change would impact the way namespaces are generated, and subsequently the imports.
this "from .error4_x_x import Error4XX" would probably need to be updated to that "from .error4XX import Error4XX" in all the python writer tests.
It wouldn't impact Graph for the error scenario, but might impact it for other scenarios, so we should kick off generation for the SDK before we merge this PR in.
@hvuhsg understood, thanks for the contributions! We'll need to run validation before changing anything else on this PR.

@andrueastman andrueastman self-assigned this May 8, 2024
@andrueastman
Copy link
Member

@baywet Looking at the diff here https://github.com/microsoftgraph/msgraph-sdk-python/compare/main..v1.0/pipelinebuild/147301

We're going to change

  • namespacing (e.g. models.o_data_errors.o_data_error to models.odata_errors.odata_error)
  • File names (e.g ios_vpp_ebook.py to ios_vpp_e_book.py)
  • Property names (e.g minimum_cpu_speed_in_mhz to minimum_cpu_speed_in_m_hz)

It may probably make sense to change this to an overload and limit this to variable name scenarios and track this for 2.0 change similar to #2495.

@baywet
Copy link
Member

baywet commented May 8, 2024

@andrueastman Thanks for validating this!
Can you provide more information about the overload option and how it'd be used in a v1?

@andrueastman
Copy link
Member

The idea would be essentially to be

  1. Add a boolean parameter(default false) to the ToSnakeCase method to enable/disable the uppercase filter
  2. Update caller instances to pass true for scenarios of parameter names and possibly file names (will need confirmation on breaking behavior) renaming so as to keep other scenarios as is (other languages can also choose to opt-in once evaluated).
  3. Create issue to remove parameter in for v2 milestone so that the behaviour is changed when we move to v2.

@baywet
Copy link
Member

baywet commented May 9, 2024

@andrueastman the named argument syntax makes changing the parameter names a breaking change foo(pos=10, forcenamed=20)
As per the file name, I think it'd would impact the imports, in which case this would represent a breaking change as well.

my suggestion at this point would be to leave this PR open in draft until we start working on a v2

@andrueastman andrueastman added this to the Kiota v2.0 milestone May 13, 2024
@andrueastman andrueastman marked this pull request as draft May 13, 2024 10:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: In Progress 🚧
Development

Successfully merging this pull request may close these issues.

Issue when converting from camel case to snake case for python generation
3 participants